Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add dbal related code from DoctrineBundle #1

Closed
wants to merge 36 commits into from

Conversation

dmaicher
Copy link

@dmaicher dmaicher commented Nov 23, 2019

#SymfonyHackday

Status:

  • create ConnectionRegistry interface + PSR-11 implementation
  • move ConnectionRegistry and Psr11ConnectionRegistry to doctrine/dbal (see Add DBAL Connection Registry dbal#3892) not sure we still need it
  • add dbal related console commands (+ tests)
  • fix profiler explain query (needs registry)
  • fix profiler list connections (needs registry)
  • add tests
  • re-apply all dbal related changes from DoctrineBundle 2.0.x done in the meantime

@stof
Copy link
Member

stof commented Nov 25, 2019

some other stuff is missing (the doctrine/dbal dependency for instance)

@dmaicher
Copy link
Author

some other stuff is missing (the doctrine/dbal dependency for instance)

@stof yes this is far from completion 😄 Also need to add/move tests

@stof
Copy link
Member

stof commented Dec 4, 2019

@dmaicher I think it might be easier to have one PR importing everything needed from the DoctrineBundle rather than splitting the import of the DI extension and other stuff (in particular given that it does not make sense to import the DI config for stuff that has not been imported yet)

@dmaicher dmaicher changed the title add dbal config & extension add dbal related code from DoctrineBundle Dec 10, 2019
@dmaicher
Copy link
Author

dmaicher commented Dec 10, 2019

@stof the basics are working 😊 I tested it using a new skeleton project:

    /**
     * @Route("/test", name="test")
     */
    public function index(Connection $connection)
    {
        $connection->ping();

        return new Response('<html><body>NICE</body></html>', 200, [
            'Content-Type' => 'text/html',
        ]);
    }

profiler

@stof
Copy link
Member

stof commented Feb 17, 2020

@dmaicher what is the status of this work ?

src/DoctrineDBALBundle.php Show resolved Hide resolved
src/Psr11ConnectionRegistry.php Outdated Show resolved Hide resolved
src/Psr11ConnectionRegistry.php Show resolved Hide resolved
@dmaicher
Copy link
Author

@dmaicher what is the status of this work ?

Thanks for your comments. I hope to get back to this within the next 1-2 weeks 😊

composer.json Show resolved Hide resolved
@dmaicher dmaicher requested a review from stof May 11, 2020 18:07
@dmaicher
Copy link
Author

dmaicher commented May 11, 2020

@stof so I propose to keep the registry inside this bundle. Can you have another look?

Next steps would be:

  • re-apply all dbal related changes from DoctrineBundle 2.0.x done in the meantime
  • test it for real on some small app including the commands and the profiler panel
  • discuss how to move forward with releasing this + using it in DoctrineBundle

/**
* Execute a SQL query and output the results.
*/
class RunSqlCommand extends DoctrineRunSqlCommand
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually since doctrine/dbal#3956 no need to extend the command anymore.

So we should require dbal 2.11 and inject an adapter from our registry to the ConnectionProvider interface

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I changed it so we support both < 2.11 and >= 2.11 without deprecations.

Or should we just require >= 2.11 already?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on the ETA for DBAL 2.11. If it is released soon enough for us to require it, I would go for that option.

Symfony 5.1 will drop support for DBAL <2.10 anyway and older DBAL version are not really maintained, so I'm fine with asking people to use an uptodate DBAL version when switching to our new bundle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants